[CORE][REFERENCE] Add bounds checking for SpaceToBatch#34667
[CORE][REFERENCE] Add bounds checking for SpaceToBatch#34667EgorDuplensky wants to merge 2 commits intoopenvinotoolkit:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds runtime-side validation for SpaceToBatch padding values to prevent invalid/overflowing padded-shape computations, and extends the template plugin reference tests with negative coverage to ensure invalid paddings throw.
Changes:
- Add non-negative and overflow guards when computing
padded_shapeinSpaceToBatchreference evaluation. - Add new negative (exception-expected) reference tests covering huge and negative padding inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/core/src/op/space_to_batch.cpp |
Adds padding validation and overflow checks during evaluate() padded-shape computation. |
src/plugins/template/tests/functional/op_reference/space_to_batch.cpp |
Adds parameterized negative tests asserting invalid padding configurations throw ov::Exception. |
You can also share your feedback on Copilot code review. Take the survey.
|
@mitruska , could you please review the core shape inference changes and tests? |
mitruska
left a comment
There was a problem hiding this comment.
Approved as the checks are valid and the output shape logic remains the same, backward compatibility is preserved.
The only suggestion is that those checks could be moved from evaluate to shape_infer.
| const auto padded_dim = | ||
| data_shape[idx] + static_cast<TVal>((*pads_begin)[idx]) + static_cast<TVal>((*pads_end)[idx]); |
There was a problem hiding this comment.
Any reason why the validation has been not extended in the shape_infer function?
The shape_infer is called during op constructor and in SpaceToBatch::evaluate, in case of Const pads and static/bounded dimensions, it could preserve from creation of such op even before execution. Also the checks could be beneficial for plugins where the shape_infer function is shared and called for dynamic case.
Related tests to cover such case are here:
openvino/src/core/tests/type_prop/space_to_batch.cpp
Lines 73 to 85 in 44967ef
There was a problem hiding this comment.
The shape infer logic uses Dimension operator+ which handles potential overflows
There was a problem hiding this comment.
I have also considered an alternative of moving the checks to a shape infer / validate step.
But I am not sure whether all those CWE detection tools will be satisfied with such approach.
Also, theoretically we can check for a buffer overflow in scope of shape_infer call but still perform some auxiliary computation in scope of 'evaluate' which could lead to some another overflow.
I mean in general shape_infer and evaluate are supposed to perform logically bound steps, but it is impossible to guarantee this.
Tickets: